Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sysctl improvements #10534

Closed
wants to merge 44 commits into from
Closed

Sysctl improvements #10534

wants to merge 44 commits into from

Conversation

maage
Copy link
Contributor

@maage maage commented May 8, 2023

Description:

When reading sysctl manuals and looking code there was some differencies. And I also wanted to convert sysctl_net_ipv4_ip_local_port_range to have support for variables. And I also wanted to try to see how jinja macro based implementations would look like.

I'm not totally happy with how it looks like.

For example setting value explicitly ensures variable=value is accepted by kernel. But it does not ensure that this value will be there at next boot. There is file based approach, but it does not manage all directories at least under RHEL etc. Files have ordering and at least for other types of tasks oscap-scanner is not able to access all files. So remediation should simulate boot in this regard to ensure there is no ordering issues or any other trickery involved. So remediation tasks should be:

  • comment out variables / wrong variable=values in files
  • add new configuration in defined place
  • ensure setup is correct as far as possible, for example when remediation file is /etc/sysctl.conf but systemd-sysctl.service does read only sysctl.d directories and it does rely on symlink /etc/sysctl.d/99-sysctl.conf -> ../sysctl.conf. What if symlink does not exist? (not done atm)
  • ensure new value can be set as wanted: sysctl -w
  • ensure new value will be set during next boot if config stays the same: /usr/lib/systemd/systemd-sysctl --cat-config or systemctl restart systemd-sysctl and check setting again (not done atm). Ansible can reload all the same but does it ensure file is what really is set during boot? I'm not sure, and bash does not do even that. SLE has own remediation file and naming ensures it is easy to override it. And even RHEL style symlink above, it is easy to override that too. So this is why in the end remediation should be rely on boot / service restart simulation to catch any an all possible trickery. This is not about sysctl issue, but for most services have this issue too. Set configuration just so, but in the end you need to enable to configuration and then ask system / runtime what their representation is and only that ensures remediation was success.

I guess sysctl.d directories should be set via product mechanism and not by each type: OVAL, bash and ansible. But it would lack all feature, which is nice for testing. At least now I have reduced it to only one test per type.

If there is need to do proper quoting, there should be also definite standards to do regexp matching. Ansible has python with per plugin variants (substing match, full content match, multiline), OVAL has tiny bit different pcre. Lastly bash has either BRE (grep, sed), ERE (grep -E, sed -E), perlre (grep -P, no sed), awk. Any jinja macro structure should support only one type of regexp format. Usual pattern is to quote regexp to make it partially literal and that should be done via specialized and tested function to ensure this macro structure does not break if there is need.

There really do need to be #10387 style support to ensure tests do not fail in envs where they are impossible.

I have not expanded sysctl test suite to match all the possible quirks, but it would probably mean many more tests. Not at #10519 level multiple dimensions but still. But I wonder if this is easiest way. It also is usually quite hard to test only one item per test as there could be quite a lot of need for example testing regexps used is okay. Does regexp testing really need to be separate each case, and is there any way to do some kind of combinations?

Another dimension of testing is this seen with comments and symlinks. Is this something that could be tested once per template? Also proper test should check situation after the fact. Now we just run OVAL eval again after remediation, but it just checks what is there, and for example commented out contents does not matter or if file contents was via symlink structure. In OVAL there is no state to check that has this filename changed from symlink to regular file or how actually has file contents changed and is it according to a what remediation should do. Tests should define domain of test and before and after remediation test suite should take copy of this domain and then compare after and before. To see if there was no side effects. For example here: /etc/sysctl.conf and sysctl.d directory contents and ansible plugin stat style dump, sysctl --all dump. This is something that is seen with sed in bash, in many cases --follow-symlinks is missing with -i. And there is comments modification with sysctl.

Next item from previous is that ansible and bash remediations should produce about the same result and those dumps done previously should also compared with each other.

One other case of harmonization is character set. Commands like grep, sed should probably always have common LC_ALL=C value.

There is missing codeclimate for OVAL. And also there is no single style to indent XML attributes. Many attributes are quite long as rule names are quite long. So any such should have their own line. Do we want to indent XML to align 2nd+ line attibute to align with 1st line attibute or have normal 4 space indent with element. And what about ordering?

Also I'm not totally happy with patches like fix: sysctl/bash: follow sysctl quirks more where in one swoop I implement multiple changes. It just gets really tedious to keep them separated after a while as after I change one aspect I need to keep multiple temporary states alive. Similar problem with style: sysctl/oval: indent 4.

Rationale:

Add sysctl_net_ipv4_ip_local_port_range. Enable sysctl rules in Fedora.

Have more robust and more base system sysctl / sysctl.d implementation following OVAL and remediation following.

bash_replace_or_append needed to have more features to accomodate all of needs I encountered.

I guess bits and peaces could be submitted as smaller PR, but this PR tries to answer question of how and why this feature could be used.

Review Hints:

At this point this probably fails in many interesting ways.

At least following ways:

Tests seem to fail because JinjaAnalysis does not understand recursive jinja macros. But as far as I can see that is only way to implement oval_list_to_set style conversion macros in jinja. As macro works as intended, I'm not going to to change it at this time. Fix available at: ComplianceAsCode/content-test-filtering#43

I don't think codeclimate complexity requirements are something to fix. Code there is just multiple possibilities. It is possible to split to functions but I don't think it is all that benefical.

I moved all commits where I needed to fix something not working in other or more strict test envs.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label May 8, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 8, 2023

Hi @maage. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label May 8, 2023
@maage maage force-pushed the sysctl-1 branch 5 times, most recently from 5a0aa92 to 6a40ef1 Compare May 11, 2023 20:37
maage added 11 commits May 12, 2023 06:43
Use str.translate method like python does.
Mostly where ever rhel8 / rhel9 is used.
bash_sed_escape_regexp is for s regexp and bash_sed_escape_replacement
is for replacement
/etc/sysctl.conf or any related directories might not exist.

Implement `bash_sysctl_test_clean` to ensure all sysctl directories and
files do exist, and there can only be configuration at `/etc/sysctl.conf`.

Implement `bash_sysctl_set_remediate_file_name`. Only one place to set
where file used to set `sysctl` remediation variables.

Implement `bash_sysctl_set_config_directories`. Per product list of
directories we are managing. Not all products manage all directories.

If there is some reason to modify this phase, now there is shared place
to do it. Use it in all `sysctl` template tests.

Also match documentation to implementation.

From: sysctl.conf(5)
...
FILES
       /etc/sysctl.d/*.conf
       /run/sysctl.d/*.conf
       /usr/local/lib/sysctl.d/*.conf
       /usr/lib/sysctl.d/*.conf
       /lib/sysctl.d/*.conf
       /etc/sysctl.conf
...
Use bash pattern to strip key from undesirables instead of invoking sed.

Some style fixes.
Baseline function striped_key just does not support complex enough regexps for key.
Sometimes data is not case insensitive and case can not be ignored.
Sometimes data does not end at word boundary as regexp word boundary \> understands it.
@maage
Copy link
Contributor Author

maage commented May 12, 2023

This seems not to be okay, and there is others.
https://artifacts.dev.testing-farm.io/b6adf7e7-8afa-46ea-8e2e-c0ad58a4d39b/

/usr/share/beakerlib/testing.sh: line 878: 12749 Segmentation fault      (core dumped) oscap xccdf eval --progress --remediate --profile xccdf_org.ssgproject.content_profile_anssi_bp28_high --report /anssi_bp28_high_remediate_report.html /ssg-cs9-ds.xml

@codeclimate
Copy link

codeclimate bot commented May 12, 2023

Code Climate has analyzed commit 436ad22 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2

Note: there is 1 critical issue.

The test coverage on the diff in this pull request is 56.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 52.4% (0.0% change).

View more on Code Climate.

maage added 7 commits May 12, 2023 11:23
Try to implement support for all quirks from manuals. Use slightly
changed logic, `bash_sed_escape_regexp`, and `quote` to achieve this.

Avoid per product quirks here by using
`bash_sysctl_set_config_directories` and
`bash_sysctl_set_remediate_file_name`.

Add LC_ALL=C to checks.

Assume sysctl values can never contain \n.

Most probably bad OVAL with too broad regex capture, or no capture at
all, so it gets all whitespace around.

sed does not allow multiline regexps and you get errors like:

sed: -e expression ComplianceAsCode#1, char 67: unterminated `s' command

use set -epu to fail if there is any issues.

Try to minimize unnecessary newlines in created files.
… directories

And thus wrong_value_usr_local_lib.fail.sh is not needed any more.

Now wrong_value_d_directory.fail.sh tests OVAL that it finds all
relevant wrong values.

During remediation test that all relevant wrong values are fixed.

I believe testing one directory at a time is not benefical usage of
testing resources. As we want to test one item, it is to test if list of
directories implemented in OVAL or in remediation are not in sync.
…ng_sysctlval_for_testing

Implemented heuristics might not work always, especially with multivalue
settings like `net.ipv4.ip_local_port_range`.

Also handle 'E226 missing whitespace around arithmetic operator'.
There is patchset to enable this:

	https://patchwork.kernel.org/project/linux-hardening/patch/[email protected]/

Some distros might have this enabled.

Add variable sysctl_kernel_perf_event_paranoid_value as variable is
required when multiple values possible.
Oval set can have only up to 2 items. This converts list to one set as
it can get quite tedious if done by hand.
maage added 22 commits May 12, 2023 11:23
When you combine xccdf variables and other format than simple int /
string, there is no generict way to implement comparison. So I decided
just to use per name comparison method.
Function sysctl_set_kernel_setting_to is not used anywhere.
Previously SYSCTLVAL == "" was variable and this did not allow to handle empty values.

Change SYSCTLVAL to be not defined if having variable and this issue is solved.
Handle emtpy string in ansible because `sysctl` module does not handle
empty string.
Change state_aide_check_attributes to ensure no prefix/suffix for
pattern.

Fix correct_with_selinux.pass.sh

Also use packages to ensure aide package is installed in tests.
Default word_boundary in bash_replace_or_append \> does not work with
*.* and like.

Replace regexp in all types to be the same. Avoid copy-paste in OVAL.

rsyslog.conf(5)
...
       Rules (selector + action)
              Every rule line consists of two fields, a selector field and an action field. These two fields are separated
              by one or more spaces or tabs. The selector field specifies a pattern of facilities and priorities belonging
              to the specified action.
...
Also drop comment or empty lines with whitespace start of line.
Newer ansible (mine 2.14) has sysctl at ansible.posix.sysctl.

But build system does not accept it:

	Found module which is not allowed:
	{'tags', 'name', 'when', 'ansible.posix.sysctl'}

and

	ERROR! couldn't resolve module/action 'ansible.posix.sysctl'. This often indicates a misspelling, missing collection, or incorrect module path.
    the replacement string in double-quoted pattern substitution does
    not undergo quote  removal,  as  it does in versions after bash-4.2
@maage
Copy link
Contributor Author

maage commented May 14, 2023

New issue from Ubuntu 22.04 / Debian

not ok 23 bash_replace_or_append - Remediate value containing forward slash
# (from function `call_bash_replace_or_append_w_format' in file test_bash_replace_or_append.bats, line 57,
#  in test file test_bash_replace_or_append.bats, line 173)
#   `call_bash_replace_or_append_w_format "$tmp_file" '^kernel.core_pattern' '|/bin/false' '%s=%s'' failed
# sed: -e expression #1, char 52: unknown option to `s'

I guess I need to extend those BATS tests, I have been running quick wrong with extra filters.

This was referenced May 16, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label May 19, 2023
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jan-cerny
Copy link
Collaborator

Closing due to inactivity. If you still want to work on this PR, please reopen it and resolve the conflicts.

@jan-cerny jan-cerny closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Used by openshift-ci bot. needs-ok-to-test Used by openshift-ci bot. needs-rebase Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants